fix(phase-ai): rank damage-removal targets by whether the damage is lethal - #6665
Conversation
…ethal Closes phase-rs#6582. EvasionRemovalPriorityPolicy ranked removal targets purely by threat value, so a fixed-damage burn spell was pointed at the biggest creature on the board — a 7/7 — even when the spell deals 3 and cannot destroy it, wasting the card. "Kills it" and "tickles it" scored identically. New building block `policies/removal_lethality.rs` (pure functions over the pending spell's own DealDamage effects and the target's runtime toughness / marked damage / indestructibility): - pending_damage_to_object: total damage the pending cast will deal to a candidate, resolved against live state (X/dynamic amounts concrete); None when it deals no damage, so the term stays inert for non-damage removal (-X/-X, destroy, exile). - damage_is_lethal: CR 120.6 / CR 704.5g marked-damage-vs-toughness, with CR 702.12b indestructible never lethal and a 0-toughness body not "killed" by the spell (its own SBA is). - lethality_bonus: +clean-kill for a lethal target; a waste penalty scaled by the surviving body (indestructible counts full toughness), capped — so a killable small target outranks a survivable large one. EvasionRemovalPriorityPolicy folds the term into its target score. This covers every direct-damage removal spell, not one card. Tests: 9 new removal-lethality tests (pure CR arithmetic + composed over a real pending-cast PolicyContext) + full 1514-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds lethality-aware removal-target scoring. Pending damage is modeled per target, lethal outcomes receive a fixed bonus, non-lethal damage receives a capped penalty, and unresolved or non-damage removal remains neutral. Tests cover damage, counters, keywords, indestructibility, source resolution, and end-to-end target selection. ChangesRemoval lethality scoring
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EvasionRemovalPriorityPolicy
participant lethality_bonus
participant PolicyContext
participant Target
EvasionRemovalPriorityPolicy->>lethality_bonus: score removal target
lethality_bonus->>PolicyContext: inspect pending DealDamage effects
PolicyContext-->>lethality_bonus: resolve damage outcome
lethality_bonus->>Target: evaluate lethality
Target-->>lethality_bonus: lethal or non-lethal result
lethality_bonus-->>EvasionRemovalPriorityPolicy: return scoring bonus
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/policies/removal_lethality.rs`:
- Around line 61-65: Update the damage aggregation in the removal-lethality
logic around the DealDamage handling and effect_targets_object to preserve each
effect’s damage_source semantics. Return a typed per-effect damage outcome,
evaluate deathtouch and wither/infect before aggregating outcomes, and ensure
those sources correctly model lethal damage against large or indestructible
creatures under the Comprehensive Rules. Use composable source-based logic and
typed enums rather than special cases, while preserving ordinary marked-damage
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 190e03a0-880f-411c-9c9c-4dc7b9ea2717
📒 Files selected for processing (5)
crates/phase-ai/src/policies/evasion_removal_priority.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/removal_lethality.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 High — removal lethality drops source-dependent damage semantics
Reviewed commit 62e465b9f49b7fb97c5ae7e4b9224586b3ab3a7e.
crates/phase-ai/src/policies/removal_lethality.rs:60-69 ignores Effect::DealDamage.damage_source, and :78-87 models lethality only as marked damage against toughness with an indestructible exception. That is not the engine's damage model:
crates/engine/src/game/effects/deal_damage.rs:1150-1215resolvesDamageSource::Targetfrom the first object target and excludes that source from the recipient slice;EachTargethas its own multi-source path.:164-188obtains deathtouch, wither, and infect from the resolved source;:519-540records deathtouch and applies wither/infect as -1/-1 counters rather than ordinary marked damage.crates/engine/src/game/sba.rs:757-780treats deathtouch damage as lethal independently of damage >= toughness, while 0 toughness is a separate SBA that indestructible does not prevent.
Consequently the policy can score the source as a recipient for DamageSource::Target, miss a source's deathtouch clean kill, and call infect/wither damage to an indestructible creature a waste even when the resulting -1/-1 counters kill it. The existing tests construct only damage_source: None (removal_lethality.rs:151-157), so none would fail for those regressions.
Please preserve source-correct recipient and damage semantics in a typed per-effect outcome before aggregation, or conservatively leave source-dependent cases neutral until that model exists. Add discriminating policy/runtime coverage for:
- default-source deathtouch against a creature too large for ordinary marked damage;
- infect and wither against an indestructible creature driven to 0 toughness;
DamageSource::TargetandDamageSource::EachTargetsource/recipient mapping, including the first Target object not being scored as a recipient.
This is a behaviorally incorrect AI-targeting result, so it cannot be enqueued as-is.
…lity Review round 1 found the lethality term dropped `Effect::DealDamage`'s `damage_source` and modelled a kill as marked-damage-vs-toughness with an indestructible exception. That is not the engine's damage model, so the policy could mis-rank three whole classes of removal. Damage results depend on the SOURCE, not just the amount (CR 120.3), so the per-effect amount is now reduced to a typed `DamageOutcome` (marked damage + -1/-1 counters + deathtouch) resolved against that effect's own source, and only then judged by `outcome_is_lethal`, which follows the same precedence as `engine::game::sba`: - CR 120.3d + CR 702.80a/702.90c: a wither/infect source marks no damage; it puts -1/-1 counters on the creature, lowering toughness (CR 122.1a). Reaching 0 toughness is CR 704.5f, which is not a destruction, so CR 702.12b indestructible does NOT prevent it — previously scored as a waste even when it cleanly killed. - CR 702.2b + CR 704.5h: a deathtouch source makes any marked damage lethal, however large the body — previously missed entirely. - CR 704.5g: the marked-damage threshold now drops with any -1/-1 counters the same spell adds. Where the damage source is not knowable while a target is still being chosen, the term reports the new `PendingDamage::Unresolved` and the policy stays neutral instead of scoring a guess: - `DamageSource::Target` — the first object target IS the source and is excluded from the recipient slice, so the object being scored may not be a recipient at all (the review's source-scored-as-recipient bug). - `DamageSource::EachTarget` — every leading target is an independent source with its own keywords and its own re-resolved amount. - `DamageSource::TriggeringSource` — bound to the triggering event's object; the engine's `extract_source_from_event` authority is crate-private and re-deriving it here would duplicate engine logic. - `EachDealsDamageEqualToPower` / `EachSourceDealsDamage` / `DamageAll` / `ApplyPostReplacementDamage` — per-source batches this layer does not model, so they bail rather than silently under-count to zero. Tests: 22 removal-lethality tests (up from 9). New discriminating cases for default-source deathtouch on an oversized body, wither and infect driving an indestructible creature to 0 toughness, counter-reduced marked-damage thresholds, waste scaled by surviving toughness, and `Target` / `EachTarget` / `TriggeringSource` / multi-source neutrality — each paired with a same-amount control that IS scored, so the assertion discriminates the source semantics rather than the target filter. Full 1527-test phase-ai lib suite passes; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed in Typed per-effect outcome before aggregation.
The waste penalty is likewise scaled by surviving toughness, so wither that doesn't finish the job is penalised as the 7/5 it left behind, not the printed 7/7. Source-dependent cases now leave the ranking alone via a new Coverage is 22 tests, up from 9, including your three cases:
Each source-dependent test is paired with a same-amount control that is scored (and each keyword test with a no-keyword control that whiffs), so the assertions discriminate the source semantics rather than the target filter — none of them pass against the previous commit. Full 1527-test |
`outcome_is_lethal` and the waste-penalty scale both needed toughness after this spell's -1/-1 counters (CR 120.3d + CR 122.1a) and each did the saturating subtraction itself. Extract `reduced_toughness` as the one authority: it is the CR 704.5g lethal-damage threshold, and clamped at 0 it is the surviving body the penalty scales by. No behaviour change — full 1527-test phase-ai lib suite passes, clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/policies/tests/removal_lethality.rs`:
- Around line 477-490: Extend the removal-lethality tests alongside
multi_source_damage_effects_stay_neutral with companion cases for
Effect::EachDealsDamageEqualToPower, Effect::DamageAll, and
Effect::ApplyPostReplacementDamage. Construct valid instances for each effect
and assert pending_for returns PendingDamage::Unresolved, preserving coverage of
the unresolved batch-damage branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9532a76d-536e-4d76-9bf5-c4f61bbc550c
📒 Files selected for processing (2)
crates/phase-ai/src/policies/removal_lethality.rscrates/phase-ai/src/policies/tests/removal_lethality.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head has two remaining test-evidence gaps before this AI-targeting change can merge.
🟡 Non-blocking
-
[MED] The lethal-removal scoring change lacks a production-policy ranking regression. Evidence:
crates/phase-ai/src/policies/evasion_removal_priority.rs:64is the production integration point, whilecrates/phase-ai/src/policies/tests/removal_lethality.rs:268-288invokeslethality_bonus/pending_damage_to_objectdirectly. Why it matters: those unit-level probes do not proveTargetSelection,EvasionRemovalPriorityPolicy, andscore_candidatescarry the bonus through to the actual target ranking. Suggested fix: add an end-to-end fixture with a 3-damage spell, a killable lower-threat creature, and a higher-threat 7/7; assert the production scorer ranks the killable target higher. -
[LOW] The batch-damage neutral-result claim is wider than its regression coverage. Evidence:
crates/phase-ai/src/policies/removal_lethality.rs:209-212groupsEachDealsDamageEqualToPower,EachSourceDealsDamage,DamageAll, andApplyPostReplacementDamage, butcrates/phase-ai/src/policies/tests/removal_lethality.rs:477-489exercises onlyEachSourceDealsDamage. Why it matters: a later change can accidentally make an untested grouped variant look lethal. Suggested fix: add one minimal valid fixture for each remaining variant and assertPendingDamage::Unresolved(and therefore a neutral bonus).
Recommendation: request changes — add the production ranking regression and the three missing grouped-variant neutral fixtures, then request another review.
…erage
Review round 2 flagged two test-evidence gaps. Both closed.
**[MED] production-path ranking regression.** The unit probes called
`lethality_bonus` / `pending_damage_to_object` directly and never proved
the bonus survives registry wiring. `burn_prefers_the_killable_body_over_
the_bigger_unkillable_threat` now drives the real path: a real Lightning
Bolt ("Lightning Bolt deals 3 damage to any target.", Oracle text
verified against Scryfall) is cast to `TargetSelection` over an opponent
2/2 and 7/7, with reach guards that the spell parses as 3 default-sourced
damage and drops no clause. It then asserts at all three production
layers — the registered `PolicyRegistry::verdicts` delta, the full
`search::score_candidates` ranking, and `choose_action` not burning the
7/7. The killable body is deliberately the LOWER threat, so threat value
alone picks the wrong target.
Fail-on-revert verified: with the `evasion_removal_priority.rs:64` wiring
neutralized, the registered policy scores the unkillable 7/7 at 2.0 and
the killable 2/2 at 1.095 and the test fails — the exact phase-rs#6582 misplay.
**[LOW] batch-damage coverage matched to the claim.** The grouped match
arm covers four variants but only `EachSourceDealsDamage` was exercised.
`batch_damage_effects_stay_neutral` is now table-driven over all four —
`EachSourceDealsDamage`, `EachDealsDamageEqualToPower`, `DamageAll`, and
`ApplyPostReplacementDamage` — asserting both `PendingDamage::Unresolved`
and a neutral bonus, so a later change can't make an untested grouped
variant look lethal. The `ApplyPostReplacementDamage` snapshot sets every
source characteristic off, so if it were ever modelled instead of bailing
it would read as plain marked damage and the assertion would fail loudly.
Tests: 23 removal-lethality tests + the production regression; full
1528-test phase-ai lib suite passes; clippy -p phase-ai --all-targets
-D warnings clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both gaps closed in [MED] production ranking regression. Fair hit — the unit probes proved the arithmetic, not the wiring.
The killable body is deliberately the lower threat, so threat value alone picks the wrong target. Fail-on-revert verified: with the [LOW] batch-damage coverage. Also correct, and the same critique CodeRabbit raised. Full 1528-test |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head d370d52d9a18c66875b058d0daa109564cec767f: the source-dependent damage handling, production ranking regression, and grouped neutral-result coverage now clear review.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Closes #6582.
Summary
Makes a direct-damage removal spell prefer a target its damage can actually kill over the biggest body on the board.
EvasionRemovalPriorityPolicyranked removal targets purely by threat value (removal_target_quality_scoregrows with the creature's value, capped at 2.0). So a fixed-damage burn spell was pointed at the largest creature — the reporter's 7/7 — even when the spell deals 3 and can't destroy it, wasting the card. Nothing in the AI compared the pending damage to the target's toughness, so "kills it" and "tickles it" scored the same and the biggest threat always won.New building block
policies/removal_lethality.rs— pure functions over the pending spell's ownDealDamageeffects and the target's runtime toughness / marked damage / indestructibility:pending_damage_to_objectXand dynamic amounts are concrete).Nonewhen it deals no damage — the signal that keeps the term inert for non-damage removal (-X/-X, destroy, exile)damage_is_lethallethality_bonus+LETHAL_BONUSfor a clean kill; a waste penalty scaled by the surviving body (indestructible counts its full toughness), capped — so a killable small target outranks a survivable large oneEvasionRemovalPriorityPolicyfoldslethality_bonusinto its existing additive target score. Because it keys on the effect AST (Effect::DealDamage) and runtime P/T, it covers every direct-damage removal spell, not one card.Files changed
crates/phase-ai/src/policies/removal_lethality.rs(new) ·policies/tests/removal_lethality.rs(new)crates/phase-ai/src/policies/evasion_removal_priority.rs(wires the term into the target score)crates/phase-ai/src/policies/mod.rs,policies/tests/mod.rsCR references
CR 120.6— damage marked accumulates; total marked ≥ toughness is lethal and destroys as an SBACR 704.5g— the lethal-damage state-based action (toughness > 0, marked ≥ toughness → destroyed)CR 702.12b— indestructible ignores the lethal-damage SBACR 120.3e— damage from a source without wither/infect is marked on the creature (multipleDealDamageeffects stack additively)Every number grep-verified against
docs/MagicCompRules.txt.Implementation method (required)
Method: manual — traced
EvasionRemovalPriorityPolicyend-to-end, confirmedplayer_impactscoresDealDamageas a flat −1.0 (no amount/toughness term), and added the lethality assessment as a reusable helper at the seam where the removal-target verdict originates.Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Performance
lethality_bonusruns only insideEvasionRemovalPriorityPolicy::score, which has already gated onWaitingFor::TargetSelection+ a harmfultargeted_object_impact. It walks the pending cast's own effect list (a handful of effects) and does oneresolve_quantityperDealDamage; no board scan, no affordability sweep, nofind_legal_targets. Returns0.0immediately when the pending spell deals the target no damage.Verification
cargo test -p phase-ai --lib— 1514 passed; 0 failed (9 new removal-lethality tests: the pure CR arithmetic — exact/short/prior-damage/indestructible/zero-toughness — plus the composedlethality_bonusover a real pending-castPolicyContext: lethal target rewarded, non-lethal 7/7 penalized below the +2.0 threat lure, indestructible penalized, non-damage removal inert)cargo clippy -p phase-ai --all-targets -- -D warnings— cleancargo fmt --all— cleancargo ai-gate— 0 FAIL. The term only moves removal target choice among creatures a damage spell is already being aimed at; CI's paired-seed AI gate re-confirms on this head.Notes
lethality_awarenesspolicy, which is about the AI's own aggression toward the opponent player (opponent_lethal_damage); this is removal-target lethality. Orthogonal scopes.evasion_removal_priority.rs, whose inline#[cfg(test)] mod testspredates this change; the new logic and its tests are in the dedicated module so they stay independently reviewable.LETHAL_BONUS/WASTE_PENALTY_MULT/WASTE_PENALTY_MAXare local calibrated constants documented in-module, matching the existingVELOCITY_BONUS_*style in the same policy; happy to promote them toPolicyPenaltieswith a paired-seed calibration if preferred.Summary by CodeRabbit